[A11y] Add Save button to Report settings single-select pages (Write capability, Notification preference, Visibility) - #94557
Conversation
…button Co-authored-by: truph01 <truph01@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
🤖 I analyzed the failing checks. There's no code failure to fix — every code/CI check passes (typecheck, all 8 The single red check is PR Author Checklist / checklist, which failed with:
This is the manual author checklist in the PR description — it can only be cleared by you checking the boxes (it's intentionally human-only, so I won't fill it out on your behalf). Once you complete the checklist, that check will pass and the PR will be green. When I first looked, several |
|
@MelvinBot Mark this PR as ready for review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d85eb1a072
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@MelvinBot Check and resolve all comments above |
…rop redundant React Compiler memoization Co-authored-by: truph01 <truph01@users.noreply.github.com>
| const {accountID: currentUserAccountID} = useCurrentUserPersonalDetails(); | ||
| const isMoneyRequest = isMoneyRequestReport(report); | ||
| const currentNotificationPreference = getReportNotificationPreference(report); | ||
| // Keep the draft undefined until the user picks a row so we always fall back to the live preference. |
There was a problem hiding this comment.
Put an empty line before the comment unless it’s on the first line of a block.
| // Keep the draft undefined until the user picks a row so we always fall back to the live preference. | |
| // Keep the draft undefined until the user picks a row so we always fall back to the live preference. |
There was a problem hiding this comment.
Done in 0f72407 — added the blank line before the comment.
| return; | ||
| } | ||
| updateRoomVisibility(report.reportID, report.visibility, newVisibility); | ||
| // Selecting Public is a sensitive change, so it still has to be confirmed before we persist it. |
There was a problem hiding this comment.
| // Selecting Public is a sensitive change, so it still has to be confirmed before we persist it. | |
| // Selecting Public is a sensitive change, so it still has to be confirmed before we persist it. |
There was a problem hiding this comment.
Done in 0f72407 — added the blank line before the comment.
|
@MelvinBot Resolve the above comments |
Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
|
The Jest Unit Tests / test (job 4) (pull_request) isn't related to this PR. |
|
@MelvinBot merge latest main here |
…ttings-save Co-authored-by: Lucien Akchoté <lakchote@users.noreply.github.com>
|
Merged the latest |
|
@MelvinBot merge latest main here |
…ttings-save Co-authored-by: Lucien Akchoté <lakchote@users.noreply.github.com>
|
Merged latest |
|
@lakchote All good now |
|
🚧 lakchote has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/lakchote in version: 9.4.46-0 🚀
|
Help site review — docs update required ✅This PR adds a Save button to the three Report/room settings single-select pages (Who can post, Notify me about new messages, Visibility), changing the flow from "select → instantly save & close" to "select → tap Save to apply." That behavior change is reflected in one help article. Draft docs PR: #97380 What I updated:
Why nothing else changed
@truph01, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
Deploy Blocker #97415 was identified to be related to this PR. |
|
@truph01 Probably this is accessibility PR. Could this be verified by A11y team? Thank you in advance |
@rushatgabhane Could you ask A11y team to verify the PRs? |
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.4.46-10 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
This is PR 1 of the batched rollout plan that extends the WCAG 3.2.2 ("On Input") fix from #92749 to the remaining single-select pages. It covers the three Report settings pages, grouped together because they live in the same directory and share the identical
SelectionList+SingleSelectListItempattern.Before this change each page persisted the change and navigated away the instant a row was selected, which is the "change of context on input" that WCAG 3.2.2 prohibits — especially disruptive for screen-reader users. The fix mirrors #92749: the selection is held in local draft state (the page stays open, the checkmark moves), and the change is only persisted + the page closed when the user taps a Save button added via
confirmButtonOptions.Pages fixed:
DynamicWriteCapabilityPage—onSelectRownow updates draftselectedWriteCapability;SavecallsupdateWriteCapability+goBack.DynamicNotificationPreferencePage—onSelectRownow updates draftselectedNotificationPreference;SavecallsupdateNotificationPreference+goBack.DynamicVisibilityPageSavepattern, but thePublicconfirmation modal is preserved: selectingPublicjust moves the checkmark, and the existing "Are you sure?" modal is now gated on Save (if the user cancels the modal, nothing is persisted and the page stays open).No
SelectionListcomponent changes; behavior is identical on web and native; optimistic/offline behavior is unchanged.Fixed Issues
$ #74834
PROPOSAL: #74834 (comment)
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review"
// Please describe what tests you performed that validates your changed worked.
Private/Restrictedand verify the page stays open until Save. SelectPublic, tap Save, and verify the "Are you sure?" confirmation modal appears; confirm → visibility saves and page closes; cancel → nothing saved and page stays open.Offline tests
Same as Tests — selecting an option and tapping Save should optimistically apply the change while offline.
QA Steps
// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review".
// Please describe what QA needs to do to validate your changes and what areas do they need to test for regressions.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videosundefined